Replace popen('uname -ap') with uname() syscall in bvar kernel_version by weim0000 · Pull Request #3517 · apache/brpc

weim0000 · 2026-09-03T13:30:41Z

What problem does this PR solve?

Problem Summary:

When a brpc server has allocated a large amount of memory, the first request to the /vars endpoint can cause a significant latency stall. This is because the kernel_version bvar variable is lazily initialized on first access, and its constructor calls popen("uname -ap") to read the kernel version.

Internally, popen() calls fork() to spawn a child process. On Linux, fork() needs to duplicate the parent process's page tables. For a server with a large memory footprint (e.g., tens of GBs), this can take hundreds of milliseconds or even longer, effectively blocking the bthread that handles the /vars request.

This caused a production incident in our environment, where the service appeared to hang when the monitoring system first scraped the /vars endpoint after the server had been running for a while with heavy memory usage.

What is changed and the side effects?

Changed:

Replace butil::read_command_output(oss, "uname -ap") (which shells out via popen → fork → exec) with the POSIX uname() syscall in src/bvar/default_variables.cpp. The uname() syscall reads the same kernel information directly via struct utsname, without creating any child process. The output format remains equivalent to uname -ap. A unit test is added in test/bvar_variable_unittest.cpp.

Side effects:

Performance effects: Eliminates the fork() overhead entirely. The uname() syscall completes in microseconds regardless of the process's memory usage, whereas the previous popen() approach could stall for 100ms+ on large-memory processes.
Breaking backward compatibility: No. The output format of the kernel_version bvar remains the same.

Copilot

🟡 Changes recommended

The new kernel_version formatting is observably different from uname -ap (newline + processor field) and the added test introduces a missing-header build risk while not exercising the actual bvar path end-to-end.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR removes the use of popen("uname -ap") (and its fork() overhead) from the lazy initialization of the kernel_version bvar by switching to the uname() syscall, aiming to eliminate first-hit /vars latency spikes in large-memory processes.

Changes:

Replace butil::read_command_output(..., "uname -ap") with uname()-based string construction for kernel_version.
Add a unit test intended to validate the uname()-derived kernel info formatting.

File summaries

File	Description
src/bvar/default_variables.cpp	Switch `kernel_version` collection from shelling out to `uname()` syscall.
test/bvar_variable_unittest.cpp	Add a new test around `uname()` output/format assumptions.

Review details

Files reviewed: 2/2 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 #include <pthread.h>                                // pthread_*
 #include <unistd.h>                                 // usleep
+#include <sys/utsname.h>                            // uname



+TEST_F(VariableTest, uname_returns_valid_kernel_info) {
+    struct utsname buf;
+    ASSERT_EQ(0, uname(&buf));
+
+    // Each field should be non-empty
+    ASSERT_GT(strlen(buf.sysname), 0u);
+    ASSERT_GT(strlen(buf.nodename), 0u);
+    ASSERT_GT(strlen(buf.release), 0u);
+    ASSERT_GT(strlen(buf.version), 0u);
+    ASSERT_GT(strlen(buf.machine), 0u);
+
+    // Build the string the same way ReadVersion does in default_variables.cpp
+    std::ostringstream oss;
+    oss << buf.sysname << ' ' << buf.nodename << ' '
+        << buf.release << ' ' << buf.version << ' '
+        << buf.machine << ' ' << buf.machine;
+    std::string content = oss.str();


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

The new kernel_version formatting is not actually equivalent to uname -ap on Linux and the added test doesn’t currently assert the behavior of the kernel_version bvar itself.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/bvar_variable_unittest.cpp:494

This new test validates that uname() returns non-empty fields, but it does not assert that the kernel_version bvar actually uses this formatting (or that its output stayed stable). Consider asserting bvar::Variable::describe_exposed("kernel_version") matches the expected string built from utsname so the test covers the behavior introduced by this PR.

    std::string content = oss.str();

    // The result should contain all key fields
    ASSERT_NE(content.find(buf.sysname), std::string::npos);
    ASSERT_NE(content.find(buf.release), std::string::npos);

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

+        struct utsname buf;
+        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version";
            return;
        }
+#if defined(__APPLE__) && (defined(__aarch64__) || defined(__arm64__))
+        const char* processor = "arm";
+#elif defined(__APPLE__) && defined(__x86_64__)
+        const char* processor = "i386";
+#else
+        const char* processor = buf.machine;
+#endif
+        std::ostringstream oss;
+        oss << buf.sysname << ' ' << buf.nodename << ' '
+            << buf.release << ' ' << buf.version << ' '
+            << buf.machine << ' ' << processor << '\n';


wwbmmm

Good fix that removes the fork()/popen stall on the /vars hot path; however the output is not strictly equivalent to uname -ap, so the no-compatibility-break claim needs to be corrected or the format adjusted.

🤖 This reply was automatically generated by brpc-oncall

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

+        struct utsname buf;
+        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version";
            return;
        }


+TEST_F(VariableTest, uname_returns_valid_kernel_info) {
+    struct utsname buf;
+    ASSERT_EQ(0, uname(&buf));


+    // Build the string the same way ReadVersion does in default_variables.cpp
+#if defined(__APPLE__) && (defined(__aarch64__) || defined(__arm64__))
+    const char* processor = "arm";
+#elif defined(__APPLE__) && defined(__x86_64__)
+    const char* processor = "i386";
+#else
+    const char* processor = buf.machine;
+#endif
+    std::ostringstream oss;
+    oss << buf.sysname << ' ' << buf.nodename << ' '
+        << buf.release << ' ' << buf.version << ' '
+        << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)
+    oss << " GNU/Linux";
+#endif
+    oss << '\n';


+    ASSERT_GT(strlen(buf.sysname), 0u);
+    ASSERT_GT(strlen(buf.nodename), 0u);
+    ASSERT_GT(strlen(buf.release), 0u);
+    ASSERT_GT(strlen(buf.version), 0u);
+    ASSERT_GT(strlen(buf.machine), 0u);


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

+        oss << buf.sysname << ' ' << buf.nodename << ' '
+            << buf.release << ' ' << buf.version << ' '
+            << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)


+    // Build the string the same way ReadVersion does in default_variables.cpp
+#if defined(__APPLE__) && (defined(__aarch64__) || defined(__arm64__))
+    const char* processor = "arm";
+#elif defined(__APPLE__) && defined(__x86_64__)
+    const char* processor = "i386";
+#else
+    const char* processor = buf.machine;
+#endif
+    std::ostringstream oss;
+    oss << buf.sysname << ' ' << buf.nodename << ' '
+        << buf.release << ' ' << buf.version << ' '
+        << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)
+    oss << " GNU/Linux";
+#endif
+    oss << '\n';
+    std::string content = oss.str();
+
+    // The result should contain all key fields
+    ASSERT_NE(content.find(buf.sysname), std::string::npos);
+    ASSERT_NE(content.find(buf.release), std::string::npos);
+    ASSERT_NE(content.find(buf.machine), std::string::npos);
+
+    // On Linux, sysname should be "Linux"; on macOS, "Darwin"
+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");


+        struct utsname buf;
+        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version";
            return;
        }


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

There is at least one concrete build/correctness issue (missing header for strerror) and a compatibility-risky formatting decision that should be resolved/clarified before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 3
Review effort level: Lite

 #include <unistd.h>                        // getpagesize
 #include <sys/types.h>
 #include <sys/resource.h>                  // getrusage
+#include <sys/utsname.h>                   // uname


+#else
+        const char* processor = buf.machine;
+#endif
+        const char* hardware_platform = buf.machine;
+        std::ostringstream oss;


    ASSERT_TRUE(destructed.load());
 }
+
+TEST_F(VariableTest, uname_returns_valid_kernel_info) {


wwbmmm · 2026-09-07T13:18:39Z

LGTM. The header-only make_kernel_version_string helper is shared by production and tests, the GNU/Linux suffix is correctly gated to Linux (non-Android), the failure log now carries errno, and my earlier unused-variable concern is resolved. No remaining blockers.

🤖 This reply was automatically generated by brpc-oncall

Copilot

🟡 Changes recommended

The new unit test’s Linux-only assertions will fail on Android builds and the newly introduced installed header/function expands the public API surface in a way that likely needs adjustment/confirmation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Lite

+        if (uname(&buf) != 0) {
+            LOG(ERROR) << "Fail to read kernel version, errno=" << errno
+                       << " (" << strerror(errno) << ")";
            return;
        }


+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");
+    ASSERT_NE(content.find("GNU/Linux"), std::string::npos);


+// from unit-test binaries (see BVAR_NOT_LINK_DEFAULT_VARIABLES in
+// variable.cpp), so keeping the formatting logic here lets tests exercise the
+// exact production formatter without depending on that object being linked.
+inline std::string make_kernel_version_string(const struct utsname& buf) {


Copilot

🔵 Needs a closer look

There are verified correctness/portability issues in the new error logging (direct strerror() use) and in the unit test’s platform guards (Linux vs Android mismatch).

Review details

Suppressed comments (2)

src/bvar/default_variables.cpp:626

strerror(errno) is used here, but default_variables.cpp does not include <string.h>/<cstring>, and strerror() is also not thread-safe. Since this file already uses PLOG, prefer PLOG(ERROR) to log the errno message without relying on strerror() directly.

        struct utsname buf;
        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version, errno=" << errno
                       << " (" << strerror(errno) << ")";
            return;

test/bvar_variable_unittest.cpp:507

The test’s platform guard doesn’t match the production formatter: make_kernel_version_string appends " GNU/Linux" only when defined(__linux__) && !defined(__ANDROID__), but the test currently asserts that substring for all __linux__ builds. This will fail on Android toolchains where __linux__ is also defined.

    // On Linux, sysname is "Linux" and the OS suffix is appended; on macOS,
    // sysname is "Darwin" and there is no OS suffix (both match `uname -ap`).
#if defined(__linux__)
    ASSERT_STREQ(buf.sysname, "Linux");
    ASSERT_NE(content.find("GNU/Linux"), std::string::npos);
#elif defined(__APPLE__)
    ASSERT_STREQ(buf.sysname, "Darwin");
    ASSERT_EQ(content.find("GNU/Linux"), std::string::npos);
#endif

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot